Codecov Report

Merging #1814 into master will increase coverage by 12.6%.
The diff coverage is 80%.

@@            Coverage Diff             @@
##           master    #1814      +/-   ##
==========================================
+ Coverage   50.44%   63.05%   +12.6%     
==========================================
  Files         448      968     +520     
  Lines      234445   292773   +58328     
  Branches    10446    12924    +2478     
==========================================
+ Hits       118268   184608   +66340     
+ Misses     113385   105366    -8019     
- Partials     2792     2799       +7

OK, added test and fixed comment (sorry I couldn't tell whether that ReadEvalError() might do some sort of non-local exit, so I hedged a bit in the comment). Hope all is well now.

Well, of course the function could "exit" non-locally via a "thrown exception" (if this was C++ code; we do something similar using setjmp). But in that case, the function won't return at all, rather the execution continues in a completely different places; so the function still won't "return" anything directly.

Good point on the terminology. I suppose a macro could have literally returned a different value, but the camel case indicated quite strongly that ReadEvalError() was not a macro. I guess I just get antsy whenever I see "error handling" functions, sorry. Thanks for moving these changes along.

I see the Travis CI failed in the test "testerror" -- which seems odd because there is no testerror.g in the gap source code tree... Let me know if there's anything I can do to lend a hand.

You can run testerror from the gap directory as follows:

cd tst/test-error
./run_error_tests.sh
cd ../test-compile
./run_compile_tests.sh

These tests aren't run by a GAP file, because they involve running lots of GAPs. I imagine you are hitting the test-error tests, which are exactly

Sorry, went to have a look, something weird is happening. Will investigate.

Ah, I see what's happened. We've renamed testerror to testspecial, and travis is being stupid.

If you rebase your commit, it should test fine. Not sure how git literate you are, but basically do a rebase. On my machine I do the following (which hopefully won't produe any errors).

git fetch --all
git rebase upstream/master
git push --force

Did I succeed? Sadly I am not that experienced with git. So I just tried your commands, and the last one failed with "fatal: The current branch fix/master/issue_917 has no upstream branch." So I messed around a bit, trying to follow the clues git was giving me, and couldn't get it to work unless I did a "git pull origin fix/master/issue_917". Then the push worked; hope it was all OK; I didn't really understand the commands I was issuing.

Sometimes git just goes crazy. I have made a new commit, #1821, which is (I hope!) just a cleaned up version of your commit. If you could just check I didn't break it (I'm fairly sure I didn't, I diffed the two versions. I did merge the 3rd commit into the 1st.

@ChrisJefferson in git advice, it's generally not a good idea to assume that upstream means what it does on your system. I'd recommend to either explain to the user how to determine the right remote name, or else specify the remote via a URL.

You are right, I gave bad advice. I suggest we move over to #1821 (where I've hopefully cleaned things up)

Sorry I wasn't experienced enough with git to know properly how to do what @ChrisJefferson was asking me to. I will switch over to pull request #1821; this one can be closed (should I do that?)

We can close by just clicking "close and comment" at the bottom. I just left it open to check you were happy, and didn't get confused by the PR just disappearing.

Don't worry, git can behave strangely, and it's sometimes hard to debug remotely (to be honest, I find it hard to debug when I'm in front of a computer using it sometimes..)

Reopening as per @fingolfin's suggestion in #1821

The diffs look as expected, so I think/hope I have executed the instructions from @fingolfin in #1821 for fixing this pull request correctly. I will now check in the further changes he requested and tests for them.

Ok, and now I have made the changes suggested by @fingolfin in his code review in #1821, along with tests for them. This was a good catch, because all of the new test cases segfault without the change. Thanks!

Actually, I don't think there is need to test the full backtrace for the second fix. So the test can live in a plain.tst file - I'd just add it to ˋtst/testinstall/callfunc.tstˋ

Right, that's what I did, except I put it in testbugfix (which doesn't test the full backtrace like test-error does), since that's where it was suggested that I put the test for my recent fix to MagmaWithOne and MagmaWithInverses with two arguments.Should I move the test for this part to testinstall?

Calling into non-function objects was mostly undocumented, unknown, and buggy in all GAP version up to and including GAP 4.8.x. So 4.9 will be the first were this is usable. Hence, I wouldn't put the test under "bugfix", but rather into the normal testsuite. Indeed, I'd always prefer to put tests there; I only place tests into testbugfix if they are one-off shots, i.e. if they only test one isolated special case, in a non-systematic fashion. Once there is a systematic tests for that area, the testbugfix one can be removed.

I'd also keep the tests as simple, short and self-contained as possible. For that reason, I really would advice against using DirectProductElement in it, as that's a high-level object, and by using it, the tests now also implicitly relies on its behavior.

Hence, the test should go into tst/testinstall/callfunc.tst, were we already test calls into non-function objects. E.g. like this, added to the end:

# test overloading CallFuncList with a procedure call
gap> cat2 := NewCategory("IsXYZ2",IsObject);;
gap> type2 := NewType(fam, cat2 and IsPositionalObjectRep);;
gap> InstallMethod(CallFuncList,[cat2,IsList],function(func,args) result:=args; end);
gap> o2 := Objectify(type2,[]);;

# test edge case: expecting a func call, but doing a proc call
gap> f := function() return o2(); end;; f();
Error, Function Calls: <func> must return a value
gap> f := function() return o2(1); end;; f();
Error, Function Calls: <func> must return a value
gap> f := function() return o2(1,2); end;; f();
Error, Function Calls: <func> must return a value
gap> f := function() return o2(1,2,3); end;; f();
Error, Function Calls: <func> must return a value
gap> f := function() return o2(1,2,3,4); end;; f();
Error, Function Calls: <func> must return a value
gap> f := function() return o2(1,2,3,4,5); end;; f();
Error, Function Calls: <func> must return a value
gap> f := function() return o2(1,2,3,4,5,6); end;; f();
Error, Function Calls: <func> must return a value
gap> f := function() return o2(1,2,3,4,5,6,7); end;; f();
Error, Function Calls: <func> must return a value

OK, I think I have covered all of your requests for changes. Thanks for the feedback.

@gwhitney Thanks. In principle, this could be merged now, but if you want to aim for "perfection", then you could still slightly improve the commit structure and messages. For example, the second commit has the commit message "Add a test to tst/test-error verifying behavior in all cases changed" which doesn't make much sense if read in isolation -- and it'll be read in isolation at times. So it's better to make that self-contained, or at the very least make clear it refers to, e.g. by saying "... in all cases changed by the previous commit.

However, in this case, I actually think it would make more sense to squash the first two commits together, and also the last three.

If you would like to try that, you can do it with an "interactive rebase". And if anything goes wrong, you can abort this operation (I'll tell you how).

So, you do this: checkout your branch. Then enter the command git rebase -i master to start the interactive rebase. This will open a file in your default editor (usually the same editor git commit opens; the default probably is vi).
In that file is a list of commits (it should show 5, namely the 5 in this PR), ordered linearly by parentage. Each commit is prefixed by a command, initially "pick"; at the bottom of the file will be a list of all possible commands. The one you'll want is "squash", which can be abbreviated to just "s".

So, you'd change the word "pick" in the second line to "s" (indicating to git that you want to squash the second commit into the second), and also in lines 4 and 5 (so that commits 4 and 5 get squashed into commit 3).

Once you changed the commands like that, you can save the file and exit the editor, at which point git starts to work. It'll open the editor two more times, to let you edit the commit message for the two resulting commits. The default will be the concatenation of the commit messages of the commits being squashed together, but you can of course freely edit those.

OK, that all worked on my laptop, but (sorry to be a dope) when I then naively tried git push origin fix/master/issue_917 I got the following output:

To https://github.com/gwhitney/gap.git
 ! [rejected]            fix/master/issue_917 -> fix/master/issue_917 (non-fast-forward)
error: failed to push some refs to 'https://github.com/gwhitney/gap.git'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Integrate the remote changes (e.g.
hint: 'git pull ...') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

How do I transmit the revised PR back to the mother ship? Thanks!

Sorry, forgot the last step: you need to pass the option --force to push

Should have guessed. But I am always slightly frightened of using excessive force, I figure there's more chance to break things... ;-)

Thanks. However, now there is a new annoyance: the second commit contains some changes/fixes that belong into the first commit, namely tweaks to tst/test-error/func-and-proc-call-trace.g and tst/test-error/func-and-proc-call-trace.g.out.

If you are still with me and not fed up by me annoying you with constant change requests, I'll describe how to fix this below. However, if you rather would not, just let me know, and I'll take care of it (I am simply telling these things to you in case you'd like to learn; it is not my intention to "punish" you for being helpful to us!!!)

Anyway, here is what you could do:

Change into your clone of the GAP repo, and to your branch. Now enter the command git reset HEAD^ tst/test-error/func-and-proc-call-trace.g*.
This causes changes which undo the changes to those tst/test-error to be put into the staging area, while the files in the work dir themselves remain unchanged (i.e. containing the changes).

So now do git commit --amend; this will display the commit message of the second commit; just save it. The result will be that the changes to tst/test-error/func-and-proc-call-trace.g* are purged from the second commit. But we still wanted them! Luckily the changed file are still in the work dir. To get them into the first commit, you can use an interactive rebase again: First we commit the changes: git commit -m fixup tst.
Now you can git rebase -i master, which this time should show you a list of three commits. We want to squash the third commit into the first. To do that, you need to swap the second and third line. I.e. the line with the commit "fixup" should be the new second line. Once that is done, also change the "pick" in that line to "f" for "fixup" (which is just like "squash", except it keeps the commit message of the first commit as-is). Now save&exit.

Git should now have transferred the changes to the first commit... And you can git push -f once again.

(If anything during this fails: no worries, git keeps backups of everything, and we can revert to those.

Kay, as requested. Impressed with your patience for enforcing crispness at the individual-commit level, as opposed to the level of merges into the repository master (which obviously have to be clean and focused). I guess if I ever embark on any serious project on the GAP codebase I will have to replay all of my changes and shuffle them into neat units before submitting a pull request, since in my experience actual development never really follows such an orderly step-by-step process as the final pair of commits in this PR would represent.

Anyhow, how about as a reward for my being a good student of git ;-), you take a quick look at the pull request to endow DirectProductElements with a working String() method (I won't put the PR number here since it isn't really a contentful cross reference)? The fact that String() is choking on DPEs is blocking me from working on my actual project (working with the authors of LOOPS and some other researchers on a better-integrated rack/quandle package that could get in shape for refereeing) on top of master.

First off, thanks for updating the commits, they are fine now :-). I also commented on the other PR.

As to your other remarks: Enforcing "crispness" on the commit level pays off on the long run, namely when you need to debug a regression via git bisect and then actually understand what was going on back then, and are able to compile, run and test (almost) every commit.

Many companies sadly fail to do this well, claiming there is no time to do things "properly" -- in my personal experience (e.g. woking as a programming freelancer during my studies, and talking about this with friends who work in IT), this very often comes with severe penalties as the projects "mature", and it's doubtful whether the time saved by rushing things vs. the time lost due to not being able to navigate the mess, is really worth it ... shrug.

Anyway, git itself is developed using these standards, and it seems to be doing quite well with it :-).

The reason I spend time trying to teach people this is selfish: I hope they'll learn to "do it properly" next time, thus saving time for everybody...

Finally, of course I don't write new code like that, I make crazy commits with horrible commit messages etc., and then once a feature is complete, I go through the mess once and sort it into something that makes sense. With some practice and routine, that doesn't waste much time, but pays of (in my experience, at least) later, as describe above.